Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: refactor specs to make it functions and expectations explicit #124

Closed
wants to merge 1 commit into from
Closed

chore: refactor specs to make it functions and expectations explicit #124

wants to merge 1 commit into from

Conversation

msuperina
Copy link
Contributor

Refactoring of how the custom commands are consumed by the test specs.
Each spec need to get a driver, load it before all the tests and unload it after all the tests.
customCommands now exports functions to be composed.
A set of functions to run the expectation in the it code, and a set of helpers to automate the writing of the it functions.

Note: although all the specs need to get a driver, there is only one instance of driver shared by all the specs (tests are run in sequence). This can change - and it will be easy to change the code with this refactoring - but so far I have had timeout issues when creating many instances, and have not figured out what is the exact issue.


cutting(4).characters().from(`123456`).startingFrom(1).shouldShow(`156.00`);
cutting(5).characters().from(`1234`).startingFrom(0).shouldShow(``);
itCutting({ cut: 0, startingFrom: 0, tested: `123456`, expected: `123,456` });
Copy link
Contributor

@ryanggrey ryanggrey Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor means we lose the bdd style syntax. The prior read as "cutting zero characters from '123456' starting from 0 should show '123,456'" and is very easy to understand the intent of the test. Whereas the new syntax requires knowing about the itCutting function and how the params are used. Is it not possible to keep the bdd type syntax by having the driver passed as a param to the customCommandsFactory? Perhaps I'm missing some other consideration that means this isn't possible.

Copy link
Contributor Author

@msuperina msuperina Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree the previous syntax was better, although it was coupling the it call to the function passed to the it. And you still needed to know about cutting, characters, from, startingFrom, shouldShow, etc... Also, the main and perhaps real disadvantage was that the cutting wrapper needed to be defined statically.
The point of this commit is to have an expectation function separate from the it call. The expectation function is defined in the beforeAll. the itCutting is cumbersome, I agree, and I can remove it and add the text explicitely here, but at least it has the advantage to provide easy composition. Also, the api about the scenario under test is quite explicit.
Any thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is my understanding that we want to be able to inject driver into each spec while being able to load before all specs and unload after all specs. I see your point about decoupling expect from it and how the proposed change uses that to inject the driver into the cutting function meaning each spec can use a different driver in the future.

I was wondering though if its possible to achieve this without changing the bdd style syntax. If we only want to be able to set the driver per description block then can driver just be injected as a param into the pre-existing customCommandsFactory? If we want to be able to set driver at the per spec level then would it be possible to add something like a usingDriver function to chainFunctions within the custom command that sets the driver for that spec (giving something like cutting(...).characters().usingDriver(...)...)? Perhaps I'm missing some consideration that means this wouldn't work though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok let me try to come up with something, I will open another PR

@msuperina
Copy link
Contributor Author

Closing after discussion in favor of #125

@msuperina msuperina closed this Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants